Codecov Report

All modified and coverable lines are covered by tests ✅

Comparison is base (e515746) 90.79% compared to head (4a70855) 90.79%.

@@           Coverage Diff           @@
##           master    #1401   +/-   ##
=======================================
  Coverage   90.79%   90.79%           
=======================================
  Files         342      342           
  Lines       11060    11060           
  Branches     1812     1812           
=======================================
  Hits        10042    10042           
  Misses        670      670           
  Partials      348      348           

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

@bjornharrtell
Why make this protected? They are almost one-liners that look fine to copy-paste. I'm hesitant because I see no real advantage of exposing these. Opening them up doesn't look like a well-thought extensibility point to me, but perhaps I'm missing something. So what's the advantage?

I agree I lack arguments for a general case here and that there is a risk that I'm catering for a pretty niche use case I've cornered myself into... That said, is not possible to replace/use the logic in these by other means, they are used in additional private parts of the FilterParser, and it's not possible to simply copy the entirety of FilterParser and adapt because it relies on internals itself.

Specifically what I would like to be able to do is to be able to override the value parsing which has become more strict (older version simply parsed to string). This perhaps only make sense when also controlling/customizing the actual queryable which I do because I have a custom Service layer not using the repository instead I roll my own ef core queries because I have an abstraction between a general resource and normalised entities. But at the same time want to use the filter parser as I do want to support the jsonapidotnet filter param/syntax.

is not possible to replace/use the logic in these by other means, they are used in additional private parts of the FilterParser, and it's not possible to simply copy the entirety of FilterParser and adapt because it relies on internals itself.

Replacement does not seem to apply here, because this PR makes the methods protected, but not virtual. So you can only call the existing implementation, not override it.

Usage should already be possible by copy/pasting the implementations. They don't rely on internals. For example, the code below compiles without error:

public sealed class MyFilterParser : FilterParser
{
    private readonly IResourceFactory _resourceFactory;

    public MyFilterParser(IResourceFactory resourceFactory)
        : base(resourceFactory)
    {
        _resourceFactory = resourceFactory;
    }

    private static Func<string, int, object> MyGetConstantValueConverterForType(Type destinationType)
    {
        return (stringValue, position) =>
        {
            try
            {
                return RuntimeTypeConverter.ConvertType(stringValue, destinationType)!;
            }
            catch (FormatException exception)
            {
                throw new QueryParseException($"Failed to convert '{stringValue}' of type 'String' to type '{destinationType.Name}'.", position, exception);
            }
        };
    }

    private Func<string, int, object> MyGetConstantValueConverterForAttribute(AttrAttribute attribute)
    {
        if (attribute is { Property.Name: nameof(Identifiable<object>.Id) })
        {
            return (stringValue, position) =>
            {
                try
                {
                    return DeObfuscateStringId(attribute.Type, stringValue);
                }
                catch (JsonApiException exception)
                {
                    throw new QueryParseException(exception.Errors[0].Detail!, position);
                }
            };
        }

        return MyGetConstantValueConverterForType(attribute.Property.PropertyType);
    }

    private object DeObfuscateStringId(ResourceType resourceType, string stringId)
    {
        IIdentifiable tempResource = _resourceFactory.CreateInstance(resourceType.ClrType);
        tempResource.StringId = stringId;
        return tempResource.GetTypedId();
    }
}

I'm not strongly against opening this up for extensibility, but if we're going to do that, additional changes are needed.

Assuming you want to be able to override the existing implementations, they need to become non-static and virtual. Also, the return value of Func<string, int, object> isn't very readable, it should be changed into a delegate, so we can document its parameters:

/// <summary>
/// Converts a constant value within a query string parameter to an <see cref="object" />.
/// </summary>
/// <param name="value">
/// The constant value to convert from.
/// </param>
/// <param name="position">The zero-based position of <paramref name="value" /> in the query string parameter value.
/// <returns>
/// The converted object instance.
/// </returns>
public delegate object ConstantValueConverter(string value, int position);

Also, it seems to me it would be sufficient to only expose GetConstantValueConverterForType instead of both.

You are right. My intention was to make them overridable. I will go the full way and see if I can actually use something like this fully with my backend before coming back to this suggestion and perhaps then I can provide a better motivation for extensibility.

Updating your branch from the latest master should fix the failing inspect-code/cleanup-code steps.

So I've made adjustments (GetConstantValueConverterForType non static and protected virtual and nothing else) and tried it out on my backend and it allows me to keep the behaviour I had in 5.1.2 and before which essentially means I have have custom Service with custom EF Core code to query from one non persistent generic (denormalized) resource entity down to specific persisted ef core entites, using the standard FilterParser logic after overriding GetConstantValueConverterForType to pass the (specific) value through as string (as it cannot be converted to an IDictionary, which is a complex/container attr/property in my generic resource entity) and then processing the filter "out of band" with some code in my custom Service and using the resulting expression with EF Core to query for actual persisted entites (which are then transformed to the base resource entity representation again and returned as resource)

If I had the opportunity to rewrite this whole thing I would probably try to model using proper EF Core inheritance and resource inheritance, as this is roughly what I'm simulating. However, it's not as simple as that - in my case I have the "base" actually being able to represent the inherited entites by gathering up the inherited entites additional properties/values into the mentioned IDictionary, and the inherited entites aren't exposed as resources at all.

While this can probably seem a bit niche/contrived/wierd use, adding this point of extensibility allows it and I don't see any apparent downsides.

Ok great. Please replace the func usage with the delegate I proposed earlier.

@bkoelman good to go hopefully! I used the delegate throughout where it appeared to make sense.

@bjornharrtell did you see my private message on gitter?

@bkoelman I see this missed the v5.5.0 release, any chance for a v5.6.0 in a (semi)near future?

Yes, but more likely v5.5.1 as not much has changed. I've focused my efforts on OpenAPI recently. Unfortunately my computer broke last weekend, it's at the repair shop right now.

@bjornharrtell This has been released now.